-
Notifications
You must be signed in to change notification settings - Fork 15.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improved event flow management related to 'will-resize' event on Windows #15695
Conversation
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Thanks for this! Looks like a good change. Would it be possible to add a test for this behaviour so we can avoid breaking it in some future refactor? |
@nornagon you're welcome! |
@neo291 looks like you'll need to rebase this – picked up some extra commits |
4ebf23d
to
dabd6c3
Compare
@codebytere you're right, sorry, my fault! I hadn't noticed that mistake, now it should be fixed, thanks. |
Seeing quite a lot of test failures on MacOS with this change; it looks fine on its face but more investigation is in order before i'd feel comfortable merging this. |
Yes I've seen them, but this change is related to Windows code only and even in the eventualty this change could affect macOS code, after a quick look to the failed tests, I think that are not related to something that can be affected by this change. |
83edf20
to
b39a43f
Compare
After some more analysis I've changed the title and commit message with a more appropriate one, as the event prevention works correctly inside the main node process also with |
Does this change make any observable difference to the behaviour of the resize event now, or is it purely to comply with windows docs recommendation? |
@nornagon no, this change doesn't make any observable difference to the behavior.
I think that is better to follow what is stated by Microsoft documentation. |
@nornagon now the comment is appropriate. |
Hmmm, so I see your point about the docs, but reading the code, I'm not sure that
So the Windows docs don't necessarily tell us the right thing to do here. I wouldn't be surprised if returning |
/cc @poiru who wrote this code in the first place :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If @poiru's OK with it then so am I.
Removing the 3-0-x and 4-0-x labels because this no longer addresses any observable issue. |
No Release Notes |
Description of Change
This is change improve the compliance with Windows event management flow for the
WM_SIZING
event.When we use the
event.preventDefault()
function we need to return true into the Windows event queue in order to signal to Windows that the event has been managed.The solution is to add the missing 'return true' into the 'prevent default' condition.
Checklist
npm test
passesRelease Notes
Notes: no-notes